inline_string: do not check if inline_string is on pmem#1100
inline_string: do not check if inline_string is on pmem#1100lukaszstolarczuk merged 1 commit intopmem:masterfrom
Conversation
KFilipek
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @igchor)
8a70604 to
3b157a7
Compare
Codecov Report
@@ Coverage Diff @@
## master #1100 +/- ##
==========================================
- Coverage 94.26% 94.23% -0.04%
==========================================
Files 51 51
Lines 5181 5221 +40
==========================================
+ Hits 4884 4920 +36
- Misses 297 301 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
lukaszstolarczuk
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @igchor)
a discussion (no related file):
pls add a sentence or two in docs
tests/inline_string/inline_string.cpp, line 249 at r2 (raw file):
}); }
perhaps test the opposite - that we can store it on DRAM...?
3b157a7 to
6612fd4
Compare
igchor
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk)
a discussion (no related file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
pls add a sentence or two in docs
Done.
tests/inline_string/inline_string.cpp, line 249 at r2 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
perhaps test the opposite - that we can store it on DRAM...?
Done.
lukaszstolarczuk
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @igchor)
include/libpmemobj++/experimental/inline_string.hpp, line 33 at r3 (raw file):
* keeps the data within the same allocation as inline_string itself. * * Unlike other containers, can be used on pmem and dram. Modifiers (like
it can be used...
include/libpmemobj++/experimental/inline_string.hpp, line 34 at r3 (raw file):
* * Unlike other containers, can be used on pmem and dram. Modifiers (like * assign() can only be called if inline string is kept on pmem).
something's wrong with the closing parenthesis in this sentence.
include/libpmemobj++/experimental/inline_string.hpp, line 357 at r3 (raw file):
* * @throw std::out_of_range if rhs is larger than capacity. * @throw pool_error is inline string is not on pmem.
is ... is (did you mean if ... is?)
tests/inline_string/inline_string.cpp, line 252 at r3 (raw file):
template <typename T> void test_dram(nvobj::pool<struct root<T>> &pop)
that's a bit unusual test, pls add some comment(s)
tests/inline_string/inline_string.cpp, line 270 at r3 (raw file):
nvobj::basic_string_view<T>(*dram_location)); dram_location->~basic_inline_string();
did you mean ~string_type? (not sure if you can call it like this)
tests/inline_string/inline_string.cpp, line 275 at r3 (raw file):
UT_ASSERT(dram_location->capacity() == string_size); UT_ASSERT(dram_location->size() == 0);
ASSERTeq
In pmemkv we have a use for keeping inline_string in DRAM. It's also safe (unlike for e.g. pmem::obj::string) since inline_string does not manage allocations.
igchor
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukaszstolarczuk)
include/libpmemobj++/experimental/inline_string.hpp, line 33 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
it can be used...
Done.
include/libpmemobj++/experimental/inline_string.hpp, line 34 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
something's wrong with the closing parenthesis in this sentence.
Done.
include/libpmemobj++/experimental/inline_string.hpp, line 357 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
is ... is(did you meanif ... is?)
Done.
tests/inline_string/inline_string.cpp, line 252 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
that's a bit unusual test, pls add some comment(s)
Done.
tests/inline_string/inline_string.cpp, line 270 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
did you mean
~string_type? (not sure if you can call it like this)
Actually yes, done ;d
tests/inline_string/inline_string.cpp, line 275 at r3 (raw file):
Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…
ASSERTeq
Done.
6612fd4 to
ed0e48c
Compare
lukaszstolarczuk
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @igchor)
karczex
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @igchor)
In pmemkv we have a use for keeping inline_string in DRAM. It's
also safe (unlike for e.g. pmem::obj::string) since inline_string
does not manage allocations.
This change is